Hierarchical refactorings #41975

jessetrinity · 2020-12-15T20:42:57Z

Adds refactorKind strings to the GetApplicableRefactorsRequest to allow editors to request a specific refactor action or set of refactors/actions. I list them here along with the old names:

"Add or remove braces in an arrow function"

"refactor.rewrite.arrow.braces.add"
"refactor.rewrite.arrow.braces.remove"

"Convert arrow function or function expression"

"refactor.rewrite.function.anonymous"
"refactor.rewrite.function.arrow"
"refactor.rewrite.function.named"

"Convert export"

"refactor.rewrite.export.named"
"refactor.rewrite.export.default"

"Convert import"

"refactor.rewrite.import.named"
"refactor.rewrite.import.namespace"

"Convert overload list to single signature"
"refactor.rewrite.function.overloadList"

"Convert parameters to destructured object"
"refactor.rewrite.parameters.toDestructured"

"Convert to template string"
"refactor.rewrite.string"

"Convert to optional chain expression"
"refactor.rewrite.expression.optionalChain"

"Extract Symbol"

"refactor.extract.constant"
"refactor.extract.function"

"Extract type"

"refactor.extract.type"
"refactor.extract.interface"
"refactor.extract.typedef"

"Generate 'get' and 'set' accessors"
"refactor.rewrite.property.generateAccessors"

"Infer function return type"
"refactor.rewrite.function.returnType"

"Move to a new file"
"refactor.move.newFile"

cc @mjbvz for input on the names. I intend to follow the VSCode/LSP style where it applies - others I made up.

getAvailableRefactors will now compare the requested refactorKind to the set of actions available to each action before checking for applicable refactors. This allows us to skip out on unnecessary work if a specific action was requested. If refactorKinds is undefined we check each refactor as usual.

A 0-index substring of a refactoKind should return every action that would complete the requested refactorKind. That is, if refactor.extract is requested, we should return all of

"refactor.extract.constant"
"refactor.extract.function"
"refactor.extract.type"
"refactor.extract.interface"
"refactor.extract.typedef"

The most specific refactorKind applicable should always be returned.

The current error behavior is to return all refactor actions available to a given refactor along with their error messages, and return no error messages if at least one refactor action is applicable. I did not intend to modify that here but it might be good to change it to return all actions that matched a request, applicable and erroring, if editors can support it.

Since I was modifying the returned actions and had to add some error messages I did some refactoring around those to make things more consistent/readable. The crux of the change is in refactorProvider.ts and protocol.ts.

typescript-bot · 2020-12-15T20:43:01Z

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

PranavSenthilnathan

Just some minor nits. Looks good otherwise.

PranavSenthilnathan · 2020-12-15T21:31:23Z

src/services/refactors/helpers.ts

+     */
+    export function refactorKindBeginsWith(known: string, requested: string | undefined): boolean {
+        if(!requested) return true;
+        return known.substr(0, requested.length) === requested;


Suggested change

return known.substr(0, requested.length) === requested;

return known.startsWith(requested);

I think I probably can't change this?

PranavSenthilnathan · 2020-12-15T21:36:04Z

src/services/refactors/helpers.ts

+     * Checks if some refactor info has refactor error info.
+     */
+    export function isRefactorErrorInfo(info: unknown): info is RefactorErrorInfo {
+        return (info as RefactorErrorInfo).error !== undefined;


Suggested change

return (info as RefactorErrorInfo).error !== undefined;

return 'error' in info;

This is what I wanted to do, but for some reason we forbid it though a lint rule:
Error: C:\Repos\TypeScript-GH\src\services\refactors\helpers.ts:14:16 no-in-operator Don't use the 'in' keyword - use 'hasProperty' to check for key presence instead

src/services/codefixes/generateAccessors.ts

src/services/types.ts

src/harness/fourslashImpl.ts

mjbvz

Thanks for looking into this. The names look good to me and the behavior you describe sounds correct to me

src/server/protocol.ts

amcasey

The protocol changes look good and I did a few other spot checks.

src/services/refactorProvider.ts

amcasey · 2020-12-17T17:55:40Z

src/services/refactors/helpers.ts

+     * Checks if string "known" begins with string "requested".
+     * Used to match requested refactorKinds with a known refactorKind.
+     */
+    export function refactorKindBeginsWith(known: string, requested: string | undefined): boolean {


Just to confirm, this doesn't require matching full hierarchy levels, right? So I can get all the extractions with "refactor.e"?

Yep. I experimented with something fancier but it didn't seem to add anything except code complexity.

it would be sorta nice if we could provide string completions for the refactor kinds, but I couldn't think of anything besides just listing them all out in a big union in the refactorKind type.

amcasey · 2020-12-17T17:56:08Z

src/services/refactors/extractType.ts

+    const extractToTypeAliasAction = {
+        name: "Extract to type alias",
+        description: getLocaleSpecificMessage(Diagnostics.Extract_to_type_alias),
+        refactorKind: "refactor.extract.type",


Is it redundant to have every refactorKind start with "refactor."?

This is an LSP naming convention that VSCode also tries to follow for it's keybinding names:
https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/#textDocument_codeAction

It felt premature to chop off the "refactor" bit without knowing how our codeAction routing will ultimately look.

src/services/refactors/moveToNewFile.ts

microsoft/TypeScript#41975

* Add support for TS's Hierarchical refactorings API microsoft/TypeScript#41975 * Add simple browser extension This change adds a new 'simple browser' extension. This extension uses a webview to render webpages directly in VS Code. We plan on using it for optionally previewing local servers in both desktop and codespaces The browser itself has a number of limitations due to the security around iframes: - It traps keyboard focus - We can't detect if a page fails to load - We can't track the current url of the iframe * Add experimental alert when the iframe is focused * Disable events on focus warning * Hooking up simple browser to opener

jessetrinity added 17 commits December 9, 2020 16:41

add hierarchical refactoring strings

a9b5140

fourslash tests

2227627

extractSymbol filters returned actions

f331c1b

move refactorKind check to utilities

ca91afd

rename parameters

265ae45

messaging for addOrRemoveBracesToArrowFunction

86d99c9

fix up inferFunctionReturnType

029a534

fix up convertArrowFunctionOrFunctionExpression

92776d4

add preferences to fourslash method

d149afc

fix up convert string

a23296f

fix up moveToNewFile

7f47126

fix lint errors

ec565e0

remove extra arrow braces diagnostics

8397605

break out tests

9c390e1

add refactor helpers

4d2da96

refactor refactors

6372062

keep list of actions

90d9c43

jessetrinity requested a review from PranavSenthilnathan December 15, 2020 20:43

typescript-bot assigned jessetrinity Dec 15, 2020

typescript-bot added the Author: Team label Dec 15, 2020

typescript-bot assigned PranavSenthilnathan Dec 15, 2020

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 15, 2020

PranavSenthilnathan approved these changes Dec 15, 2020

View reviewed changes

address PR comments

762841b

mjbvz approved these changes Dec 16, 2020

View reviewed changes

src/server/protocol.ts Outdated Show resolved Hide resolved

response protocol

d8de6c8

amcasey reviewed Dec 17, 2020

View reviewed changes

address more comments

647db26

jessetrinity merged commit 8bbef81 into microsoft:master Dec 23, 2020

mjbvz added a commit to microsoft/vscode that referenced this pull request Jan 5, 2021

Add support for TS's Hierarchical refactorings API

f4d7297

microsoft/TypeScript#41975

This was referenced Jan 8, 2021

Convert destruction #39832

Closed

Improving refactoring discoverability and UX #34930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hierarchical refactorings #41975

Hierarchical refactorings #41975

jessetrinity commented Dec 15, 2020

typescript-bot commented Dec 15, 2020

PranavSenthilnathan left a comment

PranavSenthilnathan Dec 15, 2020

jessetrinity Dec 15, 2020

PranavSenthilnathan Dec 15, 2020

jessetrinity Dec 15, 2020

mjbvz left a comment

amcasey left a comment

amcasey Dec 17, 2020

jessetrinity Dec 22, 2020

amcasey Dec 17, 2020

jessetrinity Dec 22, 2020

	return known.substr(0, requested.length) === requested;
	return known.startsWith(requested);

	return (info as RefactorErrorInfo).error !== undefined;
	return 'error' in info;

Hierarchical refactorings #41975

Hierarchical refactorings #41975

Conversation

jessetrinity commented Dec 15, 2020

typescript-bot commented Dec 15, 2020

PranavSenthilnathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjbvz left a comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment